gh-115119: Default to --with-system-libmpdec=yes by erlend-aasland · Pull Request #118539 · python/cpython

erlend-aasland · 2024-05-03T08:15:46Z

Mark _decimal as missing if libmpdecimal is not found.

Issue: Unvendor libmpdec sources #115119

📚 Documentation preview 📚: https://cpython-previews--118539.org.readthedocs.build/

Mark _decimal as missing if libmpdecimal is not found.

erlend-aasland · 2024-05-03T08:16:42Z

$ ./configure --with-system-libmpdec=no | grep -E "(decimal|mpdec)"
checking for --with-system-libmpdec... no
checking for --with-decimal-contextvar... yes
checking for decimal libmpdec machine... universal
checking for stdlib extension module _decimal... yes

$ ./configure | grep -E "(decimal|mpdec)"
checking for --with-system-libmpdec... yes
checking for libmpdec... yes
checking for --with-decimal-contextvar... yes
checking for stdlib extension module _decimal... yes
$ grep -E "_DECIMAL_(CFLAGS|LDFLAGS)=" Makefile
MODULE__DECIMAL_CFLAGS=-I/opt/homebrew/Cellar/mpdecimal/4.0.0/include
MODULE__DECIMAL_LDFLAGS=-L/opt/homebrew/Cellar/mpdecimal/4.0.0/lib -lmpdec -lm

erlend-aasland · 2024-05-03T08:19:36Z

@zware: I'm not sure if we should fall back to the internal copy if libmpdec is missing. For now, it is marked as missing if it is missing :) We could change it so configure fails if --with-system-libmpdec=yes and no mpdecimal library was found.

configure.ac

erlend-aasland · 2024-05-03T08:45:31Z

Things to consider:

Ubuntu 20.04 provides libmpdecimal 2.4 (no pkgconfig support): apt install libmpdec2
Ubuntu 22.04 provides libmpdecimal 2.5 (no pkgconfig support): apt install libmpdec3 (!)
Ubuntu 24.04 does not appear to provide libmpdecimal at all

For now, we can consider to use the bundled version for the Ubuntu CI.

zware · 2024-05-03T16:14:31Z

@zware: I'm not sure if we should fall back to the internal copy if libmpdec is missing. For now, it is marked as missing if it is missing :) We could change it so configure fails if --with-system-libmpdec=yes and no mpdecimal library was found.

Right, an explicit --with-system-libmpdec argument should be followed, failing if the system library is not there. --with-system-libmpdec=no/--without-system-libmpdec should warn, and no argument with no system library available should also warn. Whatever I said in the issue was really shorthand for "do whatever we did when we ripped out libffi because that seemed to work¹" which I think is basically what I've now laid out here :)

though that one was complicated by differing timelines on Linux and macOS which we don't need to deal with here ↩

erlend-aasland · 2024-05-05T20:59:27Z

Right, an explicit --with-system-libmpdec argument should be followed, failing if the system library is not there.

Yep

[...] --with-system-libmpdec=no/--without-system-libmpdec should warn, [..]

Yes, we need to warn for that case.

[...] and no argument with no system library available should also warn.

Hm, I think this case should fail, similar to how we handle --with-system-libmpdec=yes and no library found.

zware · 2024-05-05T21:38:19Z

[...] and no argument with no system library available should also warn.

Hm, I think this case should fail, similar to how we handle --with-system-libmpdec=yes and no library found.

I don't want a bare ./configure to start failing if mpdecimal isn't found, since that's not going to be the behavior even after the bundled library is removed. I could maybe get behind allowing the _decimal module build to fail rather than build the bundled library in that case, but even then we should also emit a warning from configure to make the issue more obvious.

erlend-aasland · 2024-05-05T21:53:26Z

Ok, how about this:

bare (no --with-system-libmpdec): warn if system library is not found, mark _decimal as missing
--with-system-libmpdec=yes: mark _decimal as missing
--with-system-libmpdec=no: use bundled version, warn that it will be removed in Python 3.15

zware

I have a couple of questions and qualms, but overall I'm okay with this as is. We can tweak it through the beta phase if needed.

.github/workflows/build.yml

Doc/using/configure.rst

configure.ac

…l of the mpdecimal sources Co-authored-by: Zachary Ware <zachary.ware@gmail.com>

…rsion check in configure

erlend-aasland · 2024-05-06T09:15:44Z

.github/workflows/posix-deps-apt.sh

    libgdbm-dev \
    libgdbm-compat-dev \
    liblzma-dev \
+    libmpdec-dev \


@hugovk: FYI, this will fail for Ubuntu 24.04, as libmpdec-dev is unavailable there.

I've been trying to figure out why that is, and have come up short (I get lost quickly in the Debian/Ubuntu development process). All I've found is a note from @doko42 in the Ubuntu python3.11 3.11.2-4 changelog, saying "Build with internal mpdecimal library, so that mpdecimal can be removed for bookworm." (and indeed, Debian bookworm does not have a libmpdec package either).

Found it:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1013395

... and:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1056785

zware · 2024-05-06T16:01:25Z

.github/workflows/posix-deps-apt.sh

    libgdbm-dev \
    libgdbm-compat-dev \
    liblzma-dev \
+    libmpdec-dev \


I've been trying to figure out why that is, and have come up short (I get lost quickly in the Debian/Ubuntu development process). All I've found is a note from @doko42 in the Ubuntu python3.11 3.11.2-4 changelog, saying "Build with internal mpdecimal library, so that mpdecimal can be removed for bookworm." (and indeed, Debian bookworm does not have a libmpdec package either).

erlend-aasland · 2024-05-06T19:16:33Z

Thanks for helping out, Zach!

Co-authored-by: Zachary Ware <zachary.ware@gmail.com>

asottile · 2024-05-11T19:29:34Z

fwiw this is going to be painful for ubuntu and debian which has removed the libmpdec system library entirely (as it was only used for building python). also this was done pretty close to the beta freeze so it definitely caught me off guard (and broke the deadsnakes builds!)

thesamesam · 2024-05-11T19:39:39Z

That seems to have been mentioned at #118539 (comment) already, but I'm not sure what the problem is. We didn't have libmpdec packaged in Gentoo until now but we've done it and now all is fine...

Why can't Debian/Ubuntu just set --with-system-libmpdec=no or --without-system-libmpdec (whichever it is) if they can't package libmpdec quickly?

erlend-aasland · 2024-05-20T02:58:51Z

@thesamesam, @asottile: we're adding a fallback to the bundled version if an external libmpdec cannot be found.

FTR, I find the recent removal of libmpdec from Ubuntu/Debian very strange.

pythongh-115119: Default to --with-system-libmpdec=yes

268d270

Mark _decimal as missing if libmpdecimal is not found.

bedevere-app bot mentioned this pull request May 3, 2024

Unvendor libmpdec sources #115119

Open

15 tasks

erlend-aasland commented May 3, 2024

View reviewed changes

configure.ac Show resolved Hide resolved

erlend-aasland added 3 commits May 5, 2024 22:38

Pull in main

45173e3

Use the same style as the surrounding code

6f020d0

Fail if --with-system-libmpdec is given but no library was found

fe56023

Address review: warn if bundled version is used

0006897

erlend-aasland added 5 commits May 6, 2024 00:21

Address review: warn if system library was not found

4f87c91

For now, build with bundled libmpdec in Ubuntu CIs

f8f4ea2

Add NEWS

8e80866

What's New

e51c1f7

Actually use bundled version if told to do so

a971a26

erlend-aasland marked this pull request as ready for review May 5, 2024 22:48

erlend-aasland requested review from corona10, ezio-melotti and hugovk as code owners May 5, 2024 22:48

bedevere-app bot added the awaiting core review label May 5, 2024

erlend-aasland requested a review from zware May 5, 2024 22:48

zware approved these changes May 6, 2024

View reviewed changes

.github/workflows/build.yml Outdated Show resolved Hide resolved

Doc/using/configure.rst Show resolved Hide resolved

Doc/using/configure.rst Show resolved Hide resolved

configure.ac Outdated Show resolved Hide resolved

configure.ac Show resolved Hide resolved

bedevere-app bot added awaiting merge and removed awaiting core review labels May 6, 2024

erlend-aasland and others added 2 commits May 6, 2024 09:02

Address review: add deprecated-removed directive regarding the remova…

e3f3e17

…l of the mpdecimal sources Co-authored-by: Zachary Ware <zachary.ware@gmail.com>

Add libmpdec-dev to CI deps, remove CI hack, and enforce mpdecimal ve…

c84f2b7

…rsion check in configure

Pull in main

161d50b

erlend-aasland marked this pull request as draft May 6, 2024 08:51

bedevere-app bot removed the awaiting merge label May 6, 2024

Make the mpdec link check more accurate, and use LIBS iso. LDFLAGS

1016f0d

erlend-aasland marked this pull request as ready for review May 6, 2024 09:12

bedevere-app bot added the awaiting core review label May 6, 2024

erlend-aasland requested a review from zware May 6, 2024 09:14

erlend-aasland commented May 6, 2024

View reviewed changes

zware approved these changes May 6, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels May 6, 2024

erlend-aasland merged commit 325a1da into python:main May 6, 2024

erlend-aasland deleted the libmpdec-default-to-system-lib branch May 6, 2024 19:16

bedevere-app bot removed the awaiting merge label May 6, 2024

SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024

pythongh-115119: Default to --with-system-libmpdec=yes (python#118539)

b62d61f

Co-authored-by: Zachary Ware <zachary.ware@gmail.com>

ned-deily mentioned this pull request May 19, 2024

Try to repair oddball test bots timing out in test_int #119166

Merged

Uh oh!

Conversation

erlend-aasland commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented May 3, 2024

Uh oh!

Uh oh!

erlend-aasland commented May 3, 2024

Uh oh!

zware commented May 3, 2024

Footnotes

Uh oh!

erlend-aasland commented May 5, 2024

Uh oh!

zware commented May 5, 2024

Uh oh!

erlend-aasland commented May 5, 2024

Uh oh!

zware left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erlend-aasland May 6, 2024

Choose a reason for hiding this comment

Uh oh!

zware May 6, 2024

Choose a reason for hiding this comment

Uh oh!

erlend-aasland May 6, 2024

Choose a reason for hiding this comment

Uh oh!

erlend-aasland May 6, 2024

Choose a reason for hiding this comment

Uh oh!

zware May 6, 2024

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented May 6, 2024

Uh oh!

asottile commented May 11, 2024

Uh oh!

thesamesam commented May 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented May 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

erlend-aasland commented May 3, 2024 •

edited

Loading

erlend-aasland commented May 3, 2024 •

edited

Loading

thesamesam commented May 11, 2024 •

edited

Loading